Expose more Sqlalchemy settings, part 1#16742
Conversation
cicdw
requested review from
desertaxle,
discdiver and
zzstoatzz
as code owners
| if self.timeout is not None: | ||
| kwargs["connect_args"] = dict(timeout=self.timeout) | ||
|
|
||
| if self.sqlalchemy_pool_size is not None: |
There was a problem hiding this comment.
I added these in #16690 so this won't affect any released behavior.
CodSpeed Performance ReportMerging #16742 will not alter performanceComparing Summary
|
zzstoatzz
left a comment
zzstoatzz
left a comment
There was a problem hiding this comment.
going to add some changes to avoid breaking the dotted key path here
zzstoatzz
force-pushed
the
sqlalchemy-settings
branch
from
1b8792f to
84dac91
Compare
zzstoatzz
force-pushed
the
sqlalchemy-settings
branch
from
9ad3818 to
0bf4d1f
Compare
zzstoatzz
left a comment
zzstoatzz
left a comment
There was a problem hiding this comment.
just to unblock since i had requested changes
Details
| def __getattribute__(self, name: str) -> Any: | ||
| if name in ["sqlalchemy_pool_size", "sqlalchemy_max_overflow"]: | ||
| warnings.warn( | ||
| f"Setting {name} has been moved to the `sqlalchemy` settings group.", | ||
| DeprecationWarning, | ||
| ) | ||
| field_name = name.replace("sqlalchemy_", "") | ||
| return getattr(super().__getattribute__("sqlalchemy"), field_name) | ||
| return super().__getattribute__(name) | ||
|
|
||
| # validators | ||
|
|
||
| @model_validator(mode="before") | ||
| @classmethod | ||
| def set_deprecated_sqlalchemy_settings_on_child_model_and_warn( | ||
| cls, values: dict[str, Any] | ||
| ) -> dict[str, Any]: | ||
| """ | ||
| Set deprecated settings on the child model. | ||
| """ | ||
| # Initialize sqlalchemy settings if not present | ||
| if "sqlalchemy" not in values: | ||
| values["sqlalchemy"] = SQLAlchemySettings() | ||
|
|
||
| if "sqlalchemy_pool_size" in values: | ||
| warnings.warn( | ||
| "`sqlalchemy_pool_size` has been moved to the `sqlalchemy` settings group as `pool_size`.", | ||
| DeprecationWarning, | ||
| ) | ||
| if "pool_size" not in values["sqlalchemy"].model_fields_set: | ||
| values["sqlalchemy"].pool_size = values["sqlalchemy_pool_size"] | ||
|
|
||
| if "sqlalchemy_max_overflow" in values: | ||
| warnings.warn( | ||
| "`sqlalchemy_max_overflow` has been moved to the `sqlalchemy` settings group as `max_overflow`.", | ||
| DeprecationWarning, | ||
| ) | ||
| if "max_overflow" not in values["sqlalchemy"].model_fields_set: | ||
| values["sqlalchemy"].max_overflow = values["sqlalchemy_max_overflow"] | ||
|
|
||
| return values |
There was a problem hiding this comment.
first part handles access to the values via the parent, second handles setting the values on the child if passed to the parent. env vars should go straight to the child via validation_alias on the child model's fields
desertaxle
left a comment
desertaxle
left a comment
There was a problem hiding this comment.
Looks good to me! I left a comment for one more test case that I think would be useful, but that's non-blocking.
| assert settings_with_new_keys.server.database.sqlalchemy.pool_size == 42 | ||
| assert settings_with_new_keys.server.database.sqlalchemy.max_overflow == 37 | ||
|
|
||
| def test_sqlalchemy_settings_migration_via_env_var( |
There was a problem hiding this comment.
Out of abundance of caution, it might be a good idea to add a test for the TOML method of setting also.
3 participants
This PR exposes additional SQLAlchemy configuration settings through Prefect settings to enable improved testing and optimization of database setups.
Note that this PR also introduces a change to our default SQLAlchemy configuration: specifically, it adds a new default of
pool_recycle=3600where we used to not use pool recycling at all.If accepted, part 2 would be to expose settings that configure the
connect_argsspecifically (which include application name, statement caching, etc.).nate
build_settings_configpublic but preserves a private copy to avoid breaking integrationssqlalchemy_*settings inprefect.tomlby removing the fields from the parentServerDatabaseSettingsand allow get/set through the parent (with aDeprecationWarning). the only way users should encounter this is by setting these settings viaprefect.toml, the env vars should always go to the right place